Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ add esm option. Add extension to imported file #311

Closed
wants to merge 1 commit into from

Conversation

Romakita
Copy link

This PR add an option to generate the .js extension required by ESM Module. It works only if the project enable "type": "module" in his package.json.

@Romakita
Copy link
Author

Hello @bencoveney
Is it possible to review this PR please ?

@bencoveney
Copy link
Owner

bencoveney commented Jun 24, 2022

Hi @Romakita

Thanks for the contribution 👍 The code changes (syntax, tests) all look good to me.

We have had some discussion previously about esm support - see:

A couple of points:

  • I haven't read up on the latest developments in supporting ESM, but IIRC always adding a .js suffix to imports doesn't work for all ESM use cases - and we would need to do something closer to whats described in Support for ES modules #178 to have a complete solution.
  • TypeScript's esm support is still experimental, so we might want to be a bit careful about how we support it at the moment https://www.typescriptlang.org/docs/handbook/esm-node.html

I've reopened #178 for discussion - I think it would be great to get a plan agreed there before we merge anything.

For anything like this where we add new options that people will then add to their config files, it will create a maintenance cost in the future (and cause us some pain if we get it wrong) so hopefully it makes sense to be a bit careful and plan that feature correctly.

@Romakita
Copy link
Author

Romakita commented Jun 24, 2022

Hello @bencoveney
thanks for your comment :)

I understand your point. we can rename the option --esm by --experimental-esm? in this case, there will be no strong commitment on your part and that brings you feedback from the community.

EDIT: Ok I see the new options --input-type. But it's more complex than I expected. You trying to detect the type of the module commonjs/module.

I haven't read up on the latest developments in supporting ESM, but IIRC always adding a .js suffix to imports doesn't work for all ESM use cases - and we would need to do something closer to whats described in #178 to have a complete solution.

In fact, your tool always import .js file to create barrel file, right ?

Today, I propose your awesome tool for any project based on the Ts.ED framework. But, the problem is around the ESM support, because your tool doesn't provide a way to generate barrels with extension, I could not keep Barreslby in the stack in the short term. And as the module works very well, I wanted to advance the subject with you rather than doing a fork.

@bencoveney
Copy link
Owner

You trying to detect the type of the module commonjs/module.

Sort of yes. My understanding at the moment is: it would be best to match the behaviour of the TypeScript compiler for determining whether a file should be treated as an ESM module, and that is roughly what the TypeScript compiler does: https://github.com/microsoft/TypeScript/blob/0ada54c006015bda2fd3b593607f58206168aa0b/src/compiler/program.ts#L845

I am open to working towards that solution incrementally - There's a note on #178 that proposes a halfway solution:

If --input-type is not provided, assume we are using CommonJS modules (until some point in the future if/when we can detect the module type automatically).
--input-type=commonjs will force Barrelsby to create CommonJS modules.
--input-type=module will force Barrelsby to create ES modules.

Would this work for your use case? I would prefer to take that route, instead of adding an experimental argument.

@BitForger interested if you have any thoughts on this, now that's popped up again.

@Romakita
Copy link
Author

@bencoveney I changed the option according to your latest comment.

But I see you working on a v3 so maybe this PR isn't longer needed :)

See you
Romain

@Romakita Romakita closed this Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants